Fix no-device crash, validate calibration, and harden against crashes#3
Merged
Conversation
…/races Reported crash: clicking Calibrate Tool with no device wedged the app in a "calibrating" state and a second click destroyed a still-joinable std::thread, aborting the process. Crash / lifecycle: - Gate the Calibrate Tool button on a selected device or recording, and block re-entry while a session is already running. - Join processingThread before reassigning the handle (Calibrate and Start Tracking) so a joinable std::thread is never destroyed. - Wrap processStreams() frame loops in try/catch so a mid-stream device unplug no longer throws out of a worker thread and aborts. - Guard pipeline.stop() behind a pipeline_started flag (and try/catch); always call tracker.shutdown() from ViewerWindow::Shutdown(); add a ViewerWindow destructor that joins all workers. - Wrap initialize()/initializeFromFile() in try/catch (GUI-thread rs2 errors). Tool calibration validation: - Reject a calibration result with more than 6 spheres or any non-finite (NaN/inf) X/Y/Z coordinate; discard it and show a modal popup telling the user the calibration was unsuccessful and to recalibrate. Enforced both in CalibrateTool (3..6 spheres) and in the UI validator. - Fix calibration out-of-bounds: empty processedFrames, varying per-frame sphere counts, empty distanceToLine, and div-by-zero -> NaN in averaging. Data races / hardening: - Add m_ToolsMutex guarding m_Tools, the index map, cur_transform and m_lTrackedTimestamp; GetToolTransform deep-copies instead of mutating the live buffer; UnionSegmentation publishes all results atomically. - Set m_bIsCurrentlyTracking before spawning the tracking thread to close a use-after-free window vs. AddTool/RemoveTool. - Guard the GUI tools vector with a mutex; worker threads use name snapshots. - Clamp numTools and per-tool sphere counts (no unbounded resize); validate tool JSON (try/catch) and ROM marker counts; clamp UDP ports; validate UDP datagram length before memcpy; reference-count socket init/deinit; initialize m_connected/m_receiveconnected; check unchecked map::find()s.
There was a problem hiding this comment.
Pull request overview
This PR hardens the RealSense Tool Tracker app against common crash/lifecycle failures (no-device calibration, joinable thread destruction, mid-stream disconnect exceptions), and adds validation/defensive checks around calibration results, tool definitions, and UDP I/O paths.
Changes:
- Adds defensive parsing/validation for tool definition JSON and clamps GUI-editable counts to prevent unsafe resizes.
- Hardens streaming lifecycle: joins worker threads before reuse, adds exception handling for RealSense pipeline start/stream loops, and makes shutdown more consistently invoked.
- Improves UDP/multi-cam robustness: reference-counted nanosockets init/deinit, datagram size validation, and safer cross-thread access to GUI-owned tool metadata.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ViewerWindow.cpp | Adds UI gating, safer thread lifecycle, UDP packet validation, tool-name snapshots, and calibration result validation. |
| src/IRToolTracking.cpp | Wraps initialize/start/stream loops with try/catch and guards pipeline stop with a started flag. |
| src/IRToolTrack.cpp | Adds tool-state mutexing, safer transform publication, and calibration edge-case handling. |
| include/ViewerWindow.h | Adds destructor-based shutdown, bounds/constants, and synchronization primitives for tools/socket init. |
| include/IRToolTracking.h | Introduces pipeline_started flag to safely gate pipeline.stop(). |
| include/IRToolTrack.h | Adds m_ToolsMutex and calibration sphere upper-bound constant. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+62
to
+66
| // Validation bounds. | ||
| static constexpr int MAX_TOOLS = 32; // upper bound for "Number of Tools" | ||
| static constexpr int MIN_SPHERES = 4; // lower bound for spheres per tool | ||
| static constexpr int MAX_SPHERES = 20; // safety cap for manual entry / ROM | ||
| static constexpr int MAX_CALIB_SPHERES = 6; // a calibration must not exceed this |
Comment on lines
+428
to
432
| // Value-initialize so no uninitialized padding goes on the wire. | ||
| TrackingData data{}; | ||
| std::copy(tool_transform.begin(), tool_transform.begin() + 3, data.position); | ||
| std::copy(tool_transform.begin() + 3, tool_transform.end(), data.quaternion); | ||
| data.toolId = static_cast<int>(id) + 1; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reported crash: clicking Calibrate Tool with no device wedged the app in a "calibrating" state and a second click destroyed a still-joinable std::thread, aborting the process.
Crash / lifecycle:
Tool calibration validation:
Data races / hardening: